Add NodeToNode Codec for PerasCertDiffusion #1658
Conversation
tbagrel1
force-pushed
the
peras-cert-codecs
branch
from
9c03877 to
124925a
Compare
tbagrel1
changed the title
|
Given what we said in the weekly meeting today, I think this should be enough to complete ticket tweag/cardano-peras#85. Let me know if I missed something :) |
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Outdated
Show resolved
Hide resolved
...sensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs
Outdated
Show resolved
Hide resolved
tbagrel1
force-pushed
the
peras-cert-codecs
branch
from
124925a to
3d9c4b1
Compare
| @@ -66,3 +70,14 @@ instance StandardHash blk => BlockSupportsPeras blk where | |||
|
|
|||
| perasCertRound = pcCertRound | |||
| perasCertBoostedBlock = pcCertBoostedBlock | |||
|
|
|||
| instance Serialise (HeaderHash blk) => Serialise (PerasCert blk) where | |||
| encode PerasCert{..} = | |||
There was a problem hiding this comment.
Nitpick: The style guide asks us to not use RecordWildCards. (I personally like them though 😅 And there are still lots of usages left from before this rule existed.)
| decodeListLenOf 2 | ||
| pcCertRound <- decode | ||
| pcCertBoostedBlock <- decode | ||
| pure $ PerasCert pcCertRound pcCertBoostedBlock |
There was a problem hiding this comment.
Minor: I think it is somewhat preferable to use NamedFieldPuns here, to make sure that we are passing the arguments to the constructor in the right order (ie in its current form, the code would also compile if we wrote pure $ PerasCert pcCertBoostedBlock pcCertRound).
| @@ -173,6 +183,30 @@ deriving newtype instance | |||
| SerialiseNodeToNode blk (GenTxId blk) => | |||
| SerialiseNodeToNode blk (WrapGenTxId blk) | |||
|
|
|||
| -- TODO: move these orphan instances elsewhere | |||
There was a problem hiding this comment.
Ah, if they are defined here, they actually aren't orphan instances (note that the orphan warning isn't disabled in this file), so this can be removed.
| @@ -173,6 +183,30 @@ deriving newtype instance | |||
| SerialiseNodeToNode blk (GenTxId blk) => | |||
| SerialiseNodeToNode blk (WrapGenTxId blk) | |||
|
|
|||
| -- TODO: move these orphan instances elsewhere | |||
| instance ConvertRawHash blk => SerialiseNodeToNode blk (Point blk) where | |||
| encodeNodeToNode _ccfg _version = encodePoint $ encodeRawHash (Proxy :: Proxy blk) | |||
There was a problem hiding this comment.
Nitpick:
| encodeNodeToNode _ccfg _version = encodePoint $ encodeRawHash (Proxy :: Proxy blk) | |
| encodeNodeToNode _ccfg _version = encodePoint $ encodeRawHash (Proxy @blk) |
Also search for "Proxy" in the style guide
| instance ConvertRawHash blk => SerialiseNodeToNode blk (PerasCert blk) where | ||
| -- Consistent with the 'Serialise' instance for 'PerasCert' defined in Ouroboros.Consensus.Block.SupportsPeras | ||
| encodeNodeToNode ccfg version PerasCert{..} = | ||
| encodeListLen 2 | ||
| <> encodeNodeToNode ccfg version pcCertRound | ||
| <> encodeNodeToNode ccfg version pcCertBoostedBlock | ||
| decodeNodeToNode ccfg version = do | ||
| decodeListLenOf 2 | ||
| pcCertRound <- decodeNodeToNode ccfg version | ||
| pcCertBoostedBlock <- decodeNodeToNode ccfg version | ||
| pure $ PerasCert pcCertRound pcCertBoostedBlock |
There was a problem hiding this comment.
You can also just defer to encode/decode here as you did for PerasRoundNo.
Closes github.com/tweag/cardano-peras/issues/85.